-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add build info metric #1332
Add build info metric #1332
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
PTAL @lilic ^^ |
main.go
Outdated
@@ -80,6 +81,7 @@ func main() { | |||
storeBuilder := store.NewBuilder() | |||
|
|||
ksmMetricsRegistry := prometheus.NewRegistry() | |||
registerBuildInfo(ksmMetricsRegistry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yeya24 Any reason why you didn't consider the existing collector? https://github.com/prometheus/common/blob/master/version/info.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice Kemal, agreed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kakkoyun I was thinking about using that directly. But that works well with promu
because promu
injects all the info via build flags to that pkg.
Right now the injected flags are added https://github.com/kubernetes/kube-state-metrics/blob/master/Makefile#L55 here, I still need to add more flags to make the VersionCollector
usable. I feel it is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I have done exactly what you said in here https://github.com/kakkoyun/observable-remote-write/blob/56b5c6dedc8eb30ece404cd4885c3aabb779d2b8/Makefile#L14
Hard to judge just which way is simpler. I guess it's a matter of preference. My preference would be obviously reusing the package from the prometheus/common
.
@lilic What do you think? Which code looks simpler to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, reusing Prometheus/common would be my preference here as well. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I will use that package. Thanks @kakkoyun for the example!
Umm, not sure how to fix CI. I tried |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Concerning CI failure, I'd suggest making sure your tooling version is the same as the CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! One suggestion only, but a nit.
The failure seems odd, but do you mind installing all the tools we use (some might need bumping so feel free to bump if you have the time) and try again.
I can also have a look tomorrow if you don't manage, thanks!!
@@ -150,6 +150,12 @@ http_request_duration_seconds_sum{handler="metrics",method="get"} 0.021113919999 | |||
http_request_duration_seconds_count{handler="metrics",method="get"} 30 | |||
``` | |||
|
|||
kube-state-metrics exposes the build info metric: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Would be good to add a few lines what those are useful for. Similiar to have we have them for the list and watch errors above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
``` | ||
|
||
`kube_state_metrics_build_info` is used to expose version and other build information. For more usage about the info pattern, | ||
please check the blog post [here](https://www.robustperception.io/exposing-the-software-version-to-prometheus). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we are allowed to link to external companies blog posts? Otherwise, agreed this blog is very useful! @brancz wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a general rule here, if this was self-promotion I would be more critical, but here it's really just a helpful resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sounds good, was on the edge. lgtm apart from the need to squash commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind just squashing your commits as the bot will merge as is, thanks! Otherwise lgtm, thanks again!
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Updated. @lilic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks for the contribution and addressing all the comments so quickly! :)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kakkoyun, lilic, yeya24 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Ben Ye yb532204897@gmail.com
What this PR does / why we need it:
This pr adds the kube_state_metrics_build_info metric as required by #1132
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1132